Bumped child process packages and open up Windows support again#64
Bumped child process packages and open up Windows support again#64WyriHaximus wants to merge 2 commits intoreactphp:0.2.xfrom
Conversation
clue
left a comment
clue
left a comment
There was a problem hiding this comment.
@WyriHaximus Thanks for looking into this! Can you provide some instructions on how to best reproduce this on Windows? Is this covered by just running the test suite on Windows? I couldn't find any references in the updated dependencies, that's why I'm wondering what changes where required upstream.
|
@clue Whoops this was supposed to be a draft PR. Wanted to do one last test run before opening it. In short all communication on the underlying dependencies goes over sockets rather then |
WyriHaximus
changed the title
WyriHaximus
force-pushed
the
support-latest-version-child-process
branch
from
165e223 to
68b50a5
Compare
|
@WyriHaximus @clue 🏓 What's the current state on this PR? |
|
I agree that supporting the latest ChildProcess component makes perfect sense, but I don't see how this currently supports Windows? Perhaps split this into a separate follow-up PR? For the reference, in case anybody's interested, here's an example how one could use socket I/O to communicate with a child process on Windows: clue/reactphp-sqlite#13 |
|
@WyriHaximus status? |
|
@CharlotteDunois right! @clue the messenger pool switched to fully using sockets in that release |
|
@WyriHaximus That's great! Let's make this actionable, what makes this PR "WIP"? |
|
@clue I can't remember 🤐 , will have a check on my windows box tomorrow |
WyriHaximus
changed the title
|
@clue Checked earlier today and it works on windows now. What do you think about adding an allowed to fail windows build on travis? |
|
@WyriHaximus That would be fantastic! See reactphp/child-process#71 for possible Travis CI config. |
|
@clue added it to this PR 👍 |
| - name: "Windows" | ||
| os: windows | ||
| language: shell # no built-in php support | ||
| before_install: |
There was a problem hiding this comment.
Looks like you may want to also update or overwrite the install and/or script instructions for Windows 👍
There was a problem hiding this comment.
yeah I missed that, will fix it later tonight 👍
WyriHaximus
force-pushed
the
support-latest-version-child-process
branch
from
871cf7d to
b06b908
Compare
|
You'll probably want to add this utility method to the tests for cross compatibility paths. https://github.com/reactphp/filesystem/pull/69/files#diff-d4c8c6dc8769324fc27cfdac19f05cafR122-R131 |
|
@CharlotteDunois yup, I'll fix all the windows build issues in this PR 🤣 |
|
@WyriHaximus status? |
WyriHaximus
force-pushed
the
support-latest-version-child-process
branch
from
b06b908 to
0a8e671
Compare
|
@CharlotteDunois just rebased and pushed it, will have a better look tomorrow |
2 participants
With the latest child process related packages adding support for windows again we can also open support for it again in this package